refactor: use neverthrow for exceptions #634

tshuli · 2020-11-15T08:02:59Z

Problem

Part of Migrate controllers and factories to TypeScript and extract services #144
This PR is the first of a series of neverthrow refactoring PRs

Solution

Refactor error throwing to instead return neverthrow err() and ok()

tshuli · 2020-11-15T08:05:08Z

@karrui continuing from #627 - could I trouble you to take a quick look if the approach is correct? If yes, will continue with the refactoring for getProcessedResponses in encrypt-submissions.server.controller and other functions with exceptions after that

karrui

general approach is correct :).

src/app/controllers/encrypt-submissions.server.controller.js

src/app/utils/encryption.ts

karrui

some changes, but as a whole correctly done

karrui · 2020-11-18T01:33:22Z

src/app/controllers/email-submissions.server.controller.js

+      )
+
+      return next()
+    } else {


nit: don't need else, since next is already returned in the block.

karrui · 2020-11-18T01:34:44Z

src/app/controllers/email-submissions.server.controller.js

-    req.attachments = mapAttachmentsFromParsedResponses(
-      req.body.parsedResponses,
-    )
-    return next()
  } else {


Can remove the else block if every branch in the if block returns

karrui · 2020-11-18T01:35:37Z

src/app/controllers/encrypt-submissions.server.controller.js

      delete req.body.responses // Prevent downstream functions from using responses by deleting it
-    } catch (err) {
+      return next()
+    } else {


no need for the else statement here too

karrui · 2020-11-18T01:37:15Z

src/app/controllers/email-submissions.server.controller.js

+      form,
+      req.body.responses,
+    )
+    if (getProcessedResponsesResult.isOk()) {


nit: prefer swapping the checks to check for errors, since that is the unusual path.

karrui · 2020-11-18T01:38:44Z

src/app/controllers/encrypt-submissions.server.controller.js

    logger.error({
      message: 'Invalid encryption',
      meta: {
        action: 'validateEncryptSubmission',
        ...createReqMeta(req),
        formId: form._id,
      },
-      error,
+      error: isEncryptedResult.error,
    })
    return res
      .status(StatusCodes.BAD_REQUEST)
      .json({ message: 'Invalid data was found. Please submit again.' })
  }

  if (req.body.responses) {


Can we swap the checks also? Makes for easier reading of the flow. Get all errors out of the way first, then the final product is a success return:

if (!req.body.responses) { return res.sendStatus(StatusCodes.BAD_REQUEST) } ... // Continue with code

karrui · 2020-11-18T01:58:57Z

src/app/utils/field-validation/index.ts

  }

  const fieldTypeEither = doFieldTypesMatch(formField, response)

  if (isLeft(fieldTypeEither)) {
-    throw new Error(fieldTypeEither.left)
+    return err(new ValidateFieldError(fieldTypeEither.left))


comment, ignore: Mixing fp-ts and neverthrow makes my brain hurt lol

karrui · 2020-11-18T02:03:40Z

src/app/utils/field-validation/index.ts

    }
  }
+  return ok(undefined)


return true here also

karrui · 2020-11-18T02:04:32Z

tests/unit/backend/utils/field-validation/attachment-validation.spec.js

-      const testFunc = () => validateField(formId, formField, response)
-      expect(testFunc).toThrow()
+      const testFunc = validateField(formId, formField, response)
+      expect(testFunc.isErr()).toBe(true)


should add expect statements to check the unwrapped error

karrui · 2020-11-18T02:04:53Z

tests/unit/backend/utils/field-validation/attachment-validation.spec.js

-      const testFunc = () => validateField(formId, formField, response)
-      expect(testFunc).not.toThrow()
+      const testFunc = validateField(formId, formField, response)
+      expect(testFunc.isOk()).toBe(true)


same here, should check for return value of ok

karrui · 2020-11-18T02:05:54Z

tests/unit/backend/utils/field-validation/attachment-validation.spec.js

@@ -34,59 +34,59 @@ describe('Attachment validation', () => {
    it('should disallow submission with no attachment if it is required', () => {
      const formField = makeField(fieldId, '1')
      const response = makeResponse(fieldId, undefined)
-      const testFunc = () => validateField(formId, formField, response)
-      expect(testFunc).toThrow()
+      const testFunc = validateField(formId, formField, response)


nit: rename testFunc -> validateResult for all the changed tests. They are not functions anymore haha

tshuli · 2020-11-18T14:22:50Z

@karrui thanks for reviewing, made edits for your re-review :)

karrui

lgtm

karrui · 2020-11-20T04:31:33Z

tests/unit/backend/utils/field-validation/checkbox-validation.spec.js

-      const moreAnswersTestFunc = () =>
-        validateField(formId, formField, moreAnswers)
-      expect(moreAnswersTestFunc).toThrow()
+      const moreAnswersTestFunc = validateField(formId, formField, moreAnswers)


missed a rename from func to result here :P

…error in log

…own result

…troller

karrui · 2020-11-26T15:05:21Z

tests/unit/backend/utils/field-validation/email-validation.spec.ts

@@ -3,6 +3,8 @@ import EmailValidator from 'src/app/utils/field-validation/validators/EmailValid
 import { BasicField } from 'src/types/field/fieldTypes'
 import { ISingleAnswerResponse } from 'src/types/response'

+import { ValidateFieldError } from '../../../../../dist/backend/app/modules/submission/submission.errors'


:( missed this buggy import here when I reviewed, only discovered when I tried to split out the tests in #742 haha. please dont import from dist in TypeScript tests!

ok got it, thanks for letting me know

karrui reviewed Nov 16, 2020

View reviewed changes

src/app/controllers/encrypt-submissions.server.controller.js Outdated Show resolved Hide resolved

src/app/controllers/encrypt-submissions.server.controller.js Outdated Show resolved Hide resolved

src/app/utils/encryption.ts Outdated Show resolved Hide resolved

tshuli force-pushed the refactor/neverthrow branch 2 times, most recently from 1c79d7d to 5f4e920 Compare November 17, 2020 12:32

tshuli marked this pull request as ready for review November 17, 2020 23:05

karrui requested changes Nov 18, 2020

View reviewed changes

tshuli force-pushed the refactor/neverthrow branch from 5f4e920 to 6ccc3c1 Compare November 18, 2020 14:02

tshuli requested a review from karrui November 18, 2020 14:22

karrui approved these changes Nov 20, 2020

View reviewed changes

tshuli added 20 commits November 23, 2020 12:41

refactor: neverthrow for checkIsEncryptedEncoding

656bd26

refactor: remove try-catch for checkIsEncryptedEncoding

c174935

fix: add .value to access neverthrown Result

d282f6e

fix: add check for nonceEncrypted

a97d120

chore: define InvalidEncodingError error class, rename, include full …

3962131

…error in log

chore: remove convert try-catch for getProcessedResponses to neverthr…

d9a7a09

…own result

refactor: neverthrow for submission service

ef7e00d

refactor: neverthrow for validateField

23dad3c

chore: update tests for validateField

3d75d32

chore: limit Error class

00a2526

chore: update validateEmailSubmission in email submissions server con…

dafb5ee

…troller

fix: return error from neverthrow error object

88227a0

chore: add ValidateFieldError class

4eddd4e

fix: ConflictError params

4cf9bf7

chore: update tests for submission.service.spec

8e423e6

chore: remove unnecessary else

6ba62c8

refactor: check for err instead of ok

7d2d00b

fix: res.sendStatus if no jsosn

de26317

docs: update error class JSDocs

5810072

docs: update comments

6ec61c8

tshuli added 4 commits November 23, 2020 12:43

refactor: return true instead of undefined if no error

7778008

chore: clean up submission.service tests

6b25cb4

chore: clean up tests

cfdcc3f

chore: update tests

9e21a7d

tshuli force-pushed the refactor/neverthrow branch from ea254ae to 9e21a7d Compare November 23, 2020 04:58

tshuli merged commit bd77a22 into develop Nov 23, 2020

tshuli deleted the refactor/neverthrow branch November 23, 2020 05:30

mantariksh mentioned this pull request Nov 24, 2020

build: release 4.47.0 #715

Merged

karrui reviewed Nov 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use neverthrow for exceptions #634

refactor: use neverthrow for exceptions #634

tshuli commented Nov 15, 2020 •

edited

Loading

tshuli commented Nov 15, 2020

karrui left a comment

karrui left a comment

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

karrui Nov 18, 2020

tshuli commented Nov 18, 2020

karrui left a comment

karrui Nov 20, 2020

karrui Nov 26, 2020

tshuli Nov 27, 2020

refactor: use neverthrow for exceptions #634

refactor: use neverthrow for exceptions #634

Conversation

tshuli commented Nov 15, 2020 • edited Loading

Problem

Solution

tshuli commented Nov 15, 2020

karrui left a comment

Choose a reason for hiding this comment

karrui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshuli commented Nov 18, 2020

karrui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshuli commented Nov 15, 2020 •

edited

Loading